Skip to content

Comments

WA-NEW-007: Stabilize create_placed_order factory#631

Open
kitcommerce wants to merge 1 commit intonextfrom
wa-new-007-stabilize-create-placed-order
Open

WA-NEW-007: Stabilize create_placed_order factory#631
kitcommerce wants to merge 1 commit intonextfrom
wa-new-007-stabilize-create-placed-order

Conversation

@kitcommerce
Copy link

Summary

Harden the create_placed_order test factory:

  • Re-apply selected shipping option to ensure Shipping#base_price is set
  • Improve UnplacedOrderError message with checkout state diagnostics
  • Fix overrides typo :update_at:updated_at

Closes #624

Client impact

None. This is a test factory change only — no production code affected.

Verify

bundle exec ruby -Icore/test core/test/services/workarea/copy_order_test.rb

Note

This branch includes WA-NEW-006 and earlier commits (stacked). Those should land first or be reviewed together.

@kitcommerce kitcommerce force-pushed the wa-new-007-stabilize-create-placed-order branch from a19d56a to 276182a Compare February 20, 2026 08:34
@kitcommerce
Copy link
Author

Heads up: this PR was unstacked/rewritten as part of #637.

I extracted the earlier commits into separate PRs (#651, #646, #647, #648, #649) and force-pushed this branch so it now contains only the WA-NEW-007 commit (based directly on next).

If you had this branch checked out locally, you will need to re-fetch/reset.

@kitcommerce kitcommerce added gate:build-pending Build gate running gate:build-passed Build gate passed and removed gate:build-pending Build gate running labels Feb 21, 2026
@kitcommerce
Copy link
Author

Dispatcher Build Gate Summary (local)

  • rubocop (diff-only): PASS (0 offenses)
  • brakeman: skipped (per repo build gate config; disabled)
  • tests (affected engines): PASS
    • core: PASS
    • admin: PASS
    • storefront: PASS

Note: test output still prints BSON Symbol deprecation warning (expected to be addressed by WA-NEW-010 / PR #635).

@kitcommerce kitcommerce added review:architecture-pending Review in progress review:simplicity-pending Review in progress review:security-pending Review in progress review:rails-conventions-pending Rails conventions review in progress labels Feb 22, 2026
@kitcommerce
Copy link
Author

⚠️ Review orchestration paused: hit an API rate limit while spawning Wave 1 reviewer agents.

No action needed from the PR author; I’ll re-dispatch Wave 1 reviews once limits clear.

@kitcommerce kitcommerce added review:architecture-pending Review in progress review:security-pending Review in progress and removed review:architecture-pending Review in progress review:simplicity-pending Review in progress review:security-pending Review in progress review:rails-conventions-pending Rails conventions review in progress labels Feb 22, 2026
@kitcommerce kitcommerce added review:security-done Review complete and removed review:security-pending Review in progress labels Feb 22, 2026
@kitcommerce
Copy link
Author

Security Review

Verdict: APPROVE (high confidence)

Summary

Changes are confined to a test factory helper and do not introduce new runtime/production attack surface. Main impact is improved determinism plus richer failure diagnostics.

Findings (non-blocking)

  • Expanded exception message may include shipping/payment errors.full_messages. In some environments those could include sensitive strings (e.g. gateway tokens) and end up in CI logs. Low risk since this is test-only, but consider redaction or gating behind an env flag if logs are broadly shared.
  • Shipping service re-application uses derived option data (ShippingOptions#find_validset_shipping_service(option.to_h)), no new input vector introduced.

@kitcommerce kitcommerce added review:architecture-done Review complete and removed review:architecture-pending Review in progress labels Feb 22, 2026
@kitcommerce
Copy link
Author

Architecture Review

Verdict: APPROVE (with notes)

Summary

This is a small, test-only change that makes the create_placed_order factory more deterministic by re-applying the selected shipping option before attempting place_order, and improves failure diagnostics when placement fails. No production/runtime surface area changes.

Notes (non-blocking)

  • The shipping-option re-application is appropriately defensive and scoped to order.requires_shipping?. It uses ShippingOptions#find_valid and re-applies the existing selection, which should reduce flakiness without changing test semantics.
  • The richer error message is helpful; as with security’s note, consider whether CI log exposure warrants omitting or redacting payment-related messages.
  • Fixing :update_at:updated_at in overrides is a straightforward correctness win.

@kitcommerce kitcommerce added review:simplicity-pending Review in progress review:rails-conventions-pending Rails conventions review in progress review:rails-conventions-done Rails conventions review complete and removed review:rails-conventions-pending Rails conventions review in progress labels Feb 22, 2026
@kitcommerce
Copy link
Author

Rails / Workarea Conventions Review

Verdict: APPROVE (high confidence)

Summary

  • Factory change follows Workarea conventions and is appropriately guarded (requires_shipping?, safe-navigation).
  • Improves diagnosability when place_order fails.
  • Fixing :update_at:updated_at looks correct.

Non-blocking nits

  • Consider flat_map { |s| s.errors.full_messages } instead of map + flatten for readability.
  • Optionally assign payment = checkout.payment once to reduce repeated safe-navigation.
  • Factory still uses update_attributes! (deprecated in newer Rails). If/when this repo is on Rails where it matters, consider update! (not blocking here).

@kitcommerce kitcommerce added review:simplicity-done Review complete and removed review:simplicity-pending Review in progress labels Feb 22, 2026
@kitcommerce
Copy link
Author

Simplicity Review

Verdict: APPROVE (with nits)

Summary

Localized, pragmatic factory hardening: re-applies the selected shipping option to ensure base shipping adjustments are present before placing the order, and improves failure diagnostics. Complexity is acceptable for test support code.

Nits (non-blocking)

  • Consider extracting the shipping re-application into a small helper method to reduce inline density.
  • Prefer checkout.shippings.flat_map { |s| s.errors.full_messages } over map + flatten.
  • Guard-clause style could reduce indentation, though current guards are safe.

Risk note

  • Re-application matches on shipping_service.name; if names drift, option will be nil and re-setting will be skipped (likely fine for a defensive factory).

@kitcommerce kitcommerce added the merge:ready All conditions met, eligible for merge label Feb 22, 2026
@kitcommerce
Copy link
Author

✅ Wave 1 Passed — Merge Ready

Wave 1 reviewers all returned APPROVE / APPROVE-with-nits:

  • architecture: ✅
  • simplicity: ✅
  • security: ✅
  • rails-conventions: ✅

Build gate: gate:build-passed

Labeling this PR merge:ready. (This is a test-only change; no client impact expected.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gate:build-passed Build gate passed merge:ready All conditions met, eligible for merge review:architecture-done Review complete review:rails-conventions-done Rails conventions review complete review:security-done Review complete review:simplicity-done Review complete

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant